Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Key handling] pass through all keys; allow specifying modifiers for validKeys[Down|Up] #1867

Merged
merged 5 commits into from
Jul 7, 2023

Conversation

nakambo
Copy link
Collaborator

@nakambo nakambo commented Jul 3, 2023

There are scenarios where it might be necessary to look at the incoming events without removing from the system queue. Currently that's impossible today on React Native macOS, since views are required to specify validKeysDown or validKeysUp, and such events are always removed from the queue.

To mitigate, let's add a new passthroughAllKeyEvents prop to RCTView. We could keep it forever (towards an interest to reduce event spam from native to JS), or we could use it towards the path to making it the default behavior (stage 1: default false, i.e. opt in, stage 2: default true, i.e. opt out, stage 3: remove, is default behavior).

  • React/Views/RCTView.h
  • React/Views/RCTView.m
  • React/Views/RCTViewManager.m

Note that this doesn't properly work with RCTUITextField (i.e. single line text fields). From what I can tell, that would need us to possibly provide a custom field editor for the window (or an event monitor). I am scoping this out for this PR.

Another peculiarity to note is regarding RCTUITextView (i.e. multi line text fields). Here, it looks like the text view itself isn't exposed to the JS (this view doesn't have a nativeTag), so there's a RCTView holding a child RCTUITextView where the former dispatches events to JS on behalf for the latter. The reason this matters (specifically for "pass through" events) is because the latter can dispatch certain events to the JS, and then depending on the super class implementation (NSTextView), it may or may not also pass the NSEvent to the next responder (i.e. parent view, i.e. RCTView). Passing the action to the next responder can cause us to send duplicate JS events for the same NSEvent. I couldn't find anything in macOS APIs to determine if the view the event was generated for is a specific view, so I am introducing a book-keeping mechanism to not send duplicate events.

Also note that on the web, it looks like input events generated by input controls (and the like) bubble to parent elements even when "handled" (as in, inserted into the text input). Given how things are currently implemented (direct events originated from NSResponder selectors), once an event is "handled" (e.g. inserted into a NSTextView), it stops propagating along the responder chain, meaning we don't simulate the web behavior (e.g. even if a parent React view opts into all keyboard events)

Introduce RCTHandledKey for specifying modifiers for validKeysDown and validKeysUp. Behavior noted in type definitions.

  • Libraries/Text/TextInput/RCTBaseTextInputView.m
  • React/Base/RCTConvert.h
  • React/Base/RCTConvert.m
  • React/Views/RCTHandledKey.h
  • React/Views/RCTHandledKey.m
  • React/Views/RCTView.h
  • React/Views/RCTView.m
  • React/Views/RCTViewKeyboardEvent.m
  • React/Views/RCTViewManager.m
  • React/Views/ScrollView/RCTScrollView.m

macOS usually does things on key down (as opposed to, say, Win32, which seems to usually does things on key up). Like RCTUITextField, passs performKeyEquivalent: to textInputDelegate so we can handle the alternate keyDown: path (e.g. Cmd+A). This will be needed for properly handling keystrokes that go through said alternate path. There are probably several other selectors that also need implementing (deleteBackward:) to full pass through every possible key, but I am leaving that for some other time.

  • Libraries/Text/TextInput/Multiline/RCTUITextView.m

Make a totally unrelated fix to RCTSwitch. In a test page where I added an on-by-default switch, I noticed the first toggle (ON->OFF) doesn't do anything. The second toggle (OFF->ON) then doesn't (expectedly) do anything. Found wrong behavior on the switch test page -- tempted to instead remove wasOn, but for now repeating the pattern in setOn:animated:

  • React/Views/RCTSwitch.m

This demonstrates the wrong\before behavior:

Flow stuff. passthroughAllKeyEvents is now a valid thing to pass to View types.

  • Libraries/Components/View/ReactNativeViewAttributes.js
  • Libraries/Components/View/ViewPropTypes.js
  • Libraries/NativeComponent/BaseViewConfig.macos.js

Update signatures for validKeysDown and validKeysUp

  • Libraries/Components/View/ViewPropTypes.js

Remove duplicated specifications on Pressable. Just use the one from View. As a benefit, future changes allow us to not have to touch Pressable anymore.

  • Libraries/Components/Pressable/Pressable.js
  • Libraries/Components/View/ViewPropTypes.js

Update test pages with passthoughAllKeyEvents and the keyboard events page with an example modifier usage.

  • packages/rn-tester/js/examples/KeyboardEventsExample/KeyboardEventsExample.js
  • packages/rn-tester/js/examples/TextInput/TextInputSharedExamples.js

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

  1. Adding an API to pass through all key down and key up events
  2. Modifying validKeysDown and validKeysUp to allow specifying modifiers
  3. Bug fix for on-by-default switch

Changelog

  • [General] [Added] Initial version

Test plan

  • Using the keyboard events test page, validate "pass through" of all events for simple view, single line text input, multi line text input. Sanity test existing (non-"pass through") behavior.
  • Using the text input test page, ordering of keyDown and keyUp events w.r.t. other events (such as keyPress -- which isn't dispatched for every key)
  • Using the switch test page, sanity test switch behaviors

…validKeys[Down|Up]

There are scenarios where it might be necessary to look at the incoming events without removing from the system queue. Currently that's impossible today on React Native macOS, since views are required to specify `validKeysDown` or `validKeysUp`, and such events are always removed from the queue.

To mitigate, let's add a new `passthroughAllKeyEvents` prop to `RCTView`. We could keep it forever (towards an interest to reduce event spam from native to JS), or we could use it towards the path to making it the default behavior (stage 1: default false, i.e. opt in, stage 2: default true, i.e. opt out, stage 3: remove, is default behavior).
- React/Views/RCTView.h
- React/Views/RCTView.m
- React/Views/RCTViewManager.m

Note that this doesn't properly work with `RCTUITextField` (i.e. single line text fields). From what I can tell, that would need us to possibly provide a custom field editor for the window. I am scoping this out for this PR.

Another peculiarity to note is regarding `RCTUITextView` (i.e. multi line text fields). Here, it looks like the text view itself isn't exposed to the JS (this view doesn't have a `nativeTag`), so there's a `RCTView` holding a child `RCTUITextView` where the former dispatches events to JS on behalf for the latter. The reason this matters (specifically for "pass through" events) is because the latter can dispatch certain events to the JS, and then depending on the super class implementation (`NSTextView`), it may or may not *also* pass the `NSEvent` to the next responder (i.e. parent view, i.e. `RCTView`). Passing the action to the next responder *can* cause us to send duplicate JS events for the same `NSEvent`. I couldn't find anything in macOS APIs to determine if the view the event was generated for is a specific view, so I am introducing a book-keeping mechanism to not send duplicate events.

Introduce `RCTHandledKey` for specifying modifiers for `validKeysDown` and `validKeysUp`. Behavior noted in type definitions.
- Libraries/Text/TextInput/RCTBaseTextInputView.m
- React/Base/RCTConvert.h
- React/Base/RCTConvert.m
- React/Views/RCTHandledKey.h
- React/Views/RCTHandledKey.m
- React/Views/RCTView.h
- React/Views/RCTView.m
- React/Views/RCTViewKeyboardEvent.m
- React/Views/RCTViewManager.m
- React/Views/ScrollView/RCTScrollView.m

macOS *usually* does things on key down (as opposed to, say, Win32, which seems to *usually* does things on key up). Like `RCTUITextField`, passs `performKeyEquivalent:` to `textInputDelegate` so we can handle the alternate `keyDown:` path (e.g. Cmd+A). This will be needed for properly handling keystrokes that go through said alternate path. There are probably several other selectors that also need implementing (`deleteBackward:`) to full pass through every possible key, but I am leaving that for some other time.
- Libraries/Text/TextInput/Multiline/RCTUITextView.m

Make a totally unrelated fix to `RCTSwitch`. In a test page where I added an on-by-default switch, I noticed the first toggle (ON->OFF) doesn't do anything. The second toggle (OFF->ON) then doesn't (expectedly) do anything. Found wrong behavior on the switch test page -- tempted to instead remove `wasOn`, but for now repeating the pattern in `setOn:animated:`
- React/Views/RCTSwitch.m

Flow stuff. `passthroughAllKeyEvents` is now a valid thing to pass to `View` types.
- Libraries/Components/View/ReactNativeViewAttributes.js
- Libraries/Components/View/ViewPropTypes.js
- Libraries/NativeComponent/BaseViewConfig.macos.js

Update signatures for `validKeysDown` and `validKeysUp`
- Libraries/Components/View/ViewPropTypes.js

Remove duplicated specifications on `Pressable`. Just use the one from `View`. As a benefit, future changes allow us to not have to touch `Pressable` anymore.
- Libraries/Components/Pressable/Pressable.js
- Libraries/Components/View/ViewPropTypes.js

Update test pages with `passthoughAllKeyEvents` and the keyboard events page with an example modifier usage.
- packages/rn-tester/js/examples/KeyboardEventsExample/KeyboardEventsExample.js
- packages/rn-tester/js/examples/TextInput/TextInputSharedExamples.js

Testing:

* Using the keyboard events test page, validate "pass through" of all events for simple view, single line text input, multi line text input. Sanity test existing (non-"pass through") behavior.
* Using the text input test page, ordering of `keyDown` and `keyUp` events w.r.t. other events (such as `keyPress` -- which isn't dispatched for every key)
* Using the switch test page, sanity test switch behaviors
@nakambo nakambo requested a review from Saadnajmi July 3, 2023 16:06
@nakambo nakambo requested a review from a team as a code owner July 3, 2023 16:06
@nakambo nakambo changed the title [Key handling] pass through all keys; allow specifying modifiers for validKeys[Down|Up] [main] [Key handling] pass through all keys; allow specifying modifiers for validKeys[Down|Up] Jul 3, 2023
React/Views/RCTSwitch.m Outdated Show resolved Hide resolved
@Saadnajmi Saadnajmi changed the title [main] [Key handling] pass through all keys; allow specifying modifiers for validKeys[Down|Up] [Key handling] pass through all keys; allow specifying modifiers for validKeys[Down|Up] Jul 6, 2023
Copy link
Collaborator

@Saadnajmi Saadnajmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments and asks for changes.

Also, Some links that might be helpful for both of us as reference

RCTUITextView implementation details (aka, why it's a child of a normal RCTUIView)

Getting TextInput to support onKeyDown/validKeysDown (and vice versa), both single and multi-line:

Libraries/Components/Pressable/Pressable.js Outdated Show resolved Hide resolved
Libraries/Components/View/ViewPropTypes.js Outdated Show resolved Hide resolved
Libraries/Components/View/ViewPropTypes.js Outdated Show resolved Hide resolved
Libraries/Components/View/ViewPropTypes.js Outdated Show resolved Hide resolved
React/Views/RCTHandledKey.h Outdated Show resolved Hide resolved
@nakambo nakambo requested a review from Saadnajmi July 7, 2023 16:11
Copy link
Collaborator

@Saadnajmi Saadnajmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, I left a few nits here and there. I'm going to do an A/B test of RNM with and without this change to compare behavior to ensure we aren't breaking :)

Libraries/Components/Pressable/Pressable.js Outdated Show resolved Hide resolved
Libraries/Text/TextInput/RCTBaseTextInputView.m Outdated Show resolved Hide resolved
React/Base/RCTConvert.h Show resolved Hide resolved
React/Base/RCTConvert.m Show resolved Hide resolved
React/Views/RCTView.h Show resolved Hide resolved
React/Views/RCTView.m Outdated Show resolved Hide resolved
React/Views/RCTHandledKey.m Outdated Show resolved Hide resolved
React/Views/RCTHandledKey.h Show resolved Hide resolved
React/Views/RCTHandledKey.h Outdated Show resolved Hide resolved
React/Views/RCTHandledKey.m Outdated Show resolved Hide resolved
@nakambo nakambo requested a review from Saadnajmi July 7, 2023 17:35
Copy link
Collaborator

@Saadnajmi Saadnajmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minus some missing diff tags, LGTM!

Libraries/Components/Pressable/Pressable.js Outdated Show resolved Hide resolved
@Saadnajmi Saadnajmi merged commit f90e387 into main Jul 7, 2023
Saadnajmi added a commit that referenced this pull request Jul 7, 2023
…validKeys[Down|Up] (#1867)

* [Key handling] pass through all keys; allow specifying modifiers for validKeys[Down|Up]

There are scenarios where it might be necessary to look at the incoming events without removing from the system queue. Currently that's impossible today on React Native macOS, since views are required to specify `validKeysDown` or `validKeysUp`, and such events are always removed from the queue.

To mitigate, let's add a new `passthroughAllKeyEvents` prop to `RCTView`. We could keep it forever (towards an interest to reduce event spam from native to JS), or we could use it towards the path to making it the default behavior (stage 1: default false, i.e. opt in, stage 2: default true, i.e. opt out, stage 3: remove, is default behavior).
- React/Views/RCTView.h
- React/Views/RCTView.m
- React/Views/RCTViewManager.m

Note that this doesn't properly work with `RCTUITextField` (i.e. single line text fields). From what I can tell, that would need us to possibly provide a custom field editor for the window. I am scoping this out for this PR.

Another peculiarity to note is regarding `RCTUITextView` (i.e. multi line text fields). Here, it looks like the text view itself isn't exposed to the JS (this view doesn't have a `nativeTag`), so there's a `RCTView` holding a child `RCTUITextView` where the former dispatches events to JS on behalf for the latter. The reason this matters (specifically for "pass through" events) is because the latter can dispatch certain events to the JS, and then depending on the super class implementation (`NSTextView`), it may or may not *also* pass the `NSEvent` to the next responder (i.e. parent view, i.e. `RCTView`). Passing the action to the next responder *can* cause us to send duplicate JS events for the same `NSEvent`. I couldn't find anything in macOS APIs to determine if the view the event was generated for is a specific view, so I am introducing a book-keeping mechanism to not send duplicate events.

Introduce `RCTHandledKey` for specifying modifiers for `validKeysDown` and `validKeysUp`. Behavior noted in type definitions.
- Libraries/Text/TextInput/RCTBaseTextInputView.m
- React/Base/RCTConvert.h
- React/Base/RCTConvert.m
- React/Views/RCTHandledKey.h
- React/Views/RCTHandledKey.m
- React/Views/RCTView.h
- React/Views/RCTView.m
- React/Views/RCTViewKeyboardEvent.m
- React/Views/RCTViewManager.m
- React/Views/ScrollView/RCTScrollView.m

macOS *usually* does things on key down (as opposed to, say, Win32, which seems to *usually* does things on key up). Like `RCTUITextField`, passs `performKeyEquivalent:` to `textInputDelegate` so we can handle the alternate `keyDown:` path (e.g. Cmd+A). This will be needed for properly handling keystrokes that go through said alternate path. There are probably several other selectors that also need implementing (`deleteBackward:`) to full pass through every possible key, but I am leaving that for some other time.
- Libraries/Text/TextInput/Multiline/RCTUITextView.m

Make a totally unrelated fix to `RCTSwitch`. In a test page where I added an on-by-default switch, I noticed the first toggle (ON->OFF) doesn't do anything. The second toggle (OFF->ON) then doesn't (expectedly) do anything. Found wrong behavior on the switch test page -- tempted to instead remove `wasOn`, but for now repeating the pattern in `setOn:animated:`
- React/Views/RCTSwitch.m

Flow stuff. `passthroughAllKeyEvents` is now a valid thing to pass to `View` types.
- Libraries/Components/View/ReactNativeViewAttributes.js
- Libraries/Components/View/ViewPropTypes.js
- Libraries/NativeComponent/BaseViewConfig.macos.js

Update signatures for `validKeysDown` and `validKeysUp`
- Libraries/Components/View/ViewPropTypes.js

Remove duplicated specifications on `Pressable`. Just use the one from `View`. As a benefit, future changes allow us to not have to touch `Pressable` anymore.
- Libraries/Components/Pressable/Pressable.js
- Libraries/Components/View/ViewPropTypes.js

Update test pages with `passthoughAllKeyEvents` and the keyboard events page with an example modifier usage.
- packages/rn-tester/js/examples/KeyboardEventsExample/KeyboardEventsExample.js
- packages/rn-tester/js/examples/TextInput/TextInputSharedExamples.js

Testing:

* Using the keyboard events test page, validate "pass through" of all events for simple view, single line text input, multi line text input. Sanity test existing (non-"pass through") behavior.
* Using the text input test page, ordering of `keyDown` and `keyUp` events w.r.t. other events (such as `keyPress` -- which isn't dispatched for every key)
* Using the switch test page, sanity test switch behaviors

* feedback

* feedback #2

* PR feedback

---------

Co-authored-by: Saad Najmi <[email protected]>
Saadnajmi added a commit that referenced this pull request Jul 7, 2023
…validKeys[Down|Up] (#1867) (#1866)

* [Key handling] pass through all keys; allow specifying modifiers for validKeys[Down|Up]

There are scenarios where it might be necessary to look at the incoming events without removing from the system queue. Currently that's impossible today on React Native macOS, since views are required to specify `validKeysDown` or `validKeysUp`, and such events are always removed from the queue.

To mitigate, let's add a new `passthroughAllKeyEvents` prop to `RCTView`. We could keep it forever (towards an interest to reduce event spam from native to JS), or we could use it towards the path to making it the default behavior (stage 1: default false, i.e. opt in, stage 2: default true, i.e. opt out, stage 3: remove, is default behavior).
- React/Views/RCTView.h
- React/Views/RCTView.m
- React/Views/RCTViewManager.m

Note that this doesn't properly work with `RCTUITextField` (i.e. single line text fields). From what I can tell, that would need us to possibly provide a custom field editor for the window. I am scoping this out for this PR.

Another peculiarity to note is regarding `RCTUITextView` (i.e. multi line text fields). Here, it looks like the text view itself isn't exposed to the JS (this view doesn't have a `nativeTag`), so there's a `RCTView` holding a child `RCTUITextView` where the former dispatches events to JS on behalf for the latter. The reason this matters (specifically for "pass through" events) is because the latter can dispatch certain events to the JS, and then depending on the super class implementation (`NSTextView`), it may or may not *also* pass the `NSEvent` to the next responder (i.e. parent view, i.e. `RCTView`). Passing the action to the next responder *can* cause us to send duplicate JS events for the same `NSEvent`. I couldn't find anything in macOS APIs to determine if the view the event was generated for is a specific view, so I am introducing a book-keeping mechanism to not send duplicate events.

Introduce `RCTHandledKey` for specifying modifiers for `validKeysDown` and `validKeysUp`. Behavior noted in type definitions.
- Libraries/Text/TextInput/RCTBaseTextInputView.m
- React/Base/RCTConvert.h
- React/Base/RCTConvert.m
- React/Views/RCTHandledKey.h
- React/Views/RCTHandledKey.m
- React/Views/RCTView.h
- React/Views/RCTView.m
- React/Views/RCTViewKeyboardEvent.m
- React/Views/RCTViewManager.m
- React/Views/ScrollView/RCTScrollView.m

macOS *usually* does things on key down (as opposed to, say, Win32, which seems to *usually* does things on key up). Like `RCTUITextField`, passs `performKeyEquivalent:` to `textInputDelegate` so we can handle the alternate `keyDown:` path (e.g. Cmd+A). This will be needed for properly handling keystrokes that go through said alternate path. There are probably several other selectors that also need implementing (`deleteBackward:`) to full pass through every possible key, but I am leaving that for some other time.
- Libraries/Text/TextInput/Multiline/RCTUITextView.m

Make a totally unrelated fix to `RCTSwitch`. In a test page where I added an on-by-default switch, I noticed the first toggle (ON->OFF) doesn't do anything. The second toggle (OFF->ON) then doesn't (expectedly) do anything. Found wrong behavior on the switch test page -- tempted to instead remove `wasOn`, but for now repeating the pattern in `setOn:animated:`
- React/Views/RCTSwitch.m

Flow stuff. `passthroughAllKeyEvents` is now a valid thing to pass to `View` types.
- Libraries/Components/View/ReactNativeViewAttributes.js
- Libraries/Components/View/ViewPropTypes.js
- Libraries/NativeComponent/BaseViewConfig.macos.js

Update signatures for `validKeysDown` and `validKeysUp`
- Libraries/Components/View/ViewPropTypes.js

Remove duplicated specifications on `Pressable`. Just use the one from `View`. As a benefit, future changes allow us to not have to touch `Pressable` anymore.
- Libraries/Components/Pressable/Pressable.js
- Libraries/Components/View/ViewPropTypes.js

Update test pages with `passthoughAllKeyEvents` and the keyboard events page with an example modifier usage.
- packages/rn-tester/js/examples/KeyboardEventsExample/KeyboardEventsExample.js
- packages/rn-tester/js/examples/TextInput/TextInputSharedExamples.js

Testing:

* Using the keyboard events test page, validate "pass through" of all events for simple view, single line text input, multi line text input. Sanity test existing (non-"pass through") behavior.
* Using the text input test page, ordering of `keyDown` and `keyUp` events w.r.t. other events (such as `keyPress` -- which isn't dispatched for every key)
* Using the switch test page, sanity test switch behaviors

* feedback

* feedback #2

* PR feedback

---------

Co-authored-by: Saad Najmi <[email protected]>
@nakambo nakambo deleted the nakambo/key-handling branch July 7, 2023 23:16
nakambo added a commit that referenced this pull request Oct 8, 2024
performKeyEquivalent is [documented](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/EventOverview/EventArchitecture/EventArchitecture.html#//apple_ref/doc/uid/10000060i-CH3-SW10) to be called even if a view isn't first responder (this is for example how "Enter" binds to the default button on a dialog, for example -- the default button doesn't need to be focused to respond to Enter), but since we override performKeyEquivalent to properly support certain keyboard shortcuts (like noted in #1867), the previous assumption on the actual use of performKeyEquivalent no longer holds -- it's *only* for keystrokes sent in focused state, so add that check.
nakambo added a commit that referenced this pull request Oct 8, 2024
performKeyEquivalent is [documented](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/EventOverview/EventArchitecture/EventArchitecture.html#//apple_ref/doc/uid/10000060i-CH3-SW10) to be called even if a view isn't first responder (this is for example how "Enter" binds to the default button on a dialog, for example -- the default button doesn't need to be focused to respond to Enter), but since we override performKeyEquivalent to properly support certain keyboard shortcuts (like noted in #1867), the previous assumption on the actual use of performKeyEquivalent no longer holds -- it's *only* for keystrokes sent in focused state, so add that check.
nakambo added a commit that referenced this pull request Oct 8, 2024
performKeyEquivalent is [documented](https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/EventOverview/EventArchitecture/EventArchitecture.html#//apple_ref/doc/uid/10000060i-CH3-SW10) to be called even if a view isn't first responder (this is for example how "Enter" binds to the default button on a dialog, for example -- the default button doesn't need to be focused to respond to Enter), but since we override performKeyEquivalent to properly support certain keyboard shortcuts (like noted in #1867), the previous assumption on the actual use of performKeyEquivalent no longer holds -- it's *only* for keystrokes sent in focused state, so add that check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants